allow exporting remote collection #2813
Conversation
Reviewer's Guide by SourceryThis pull request allows exporting remote collections. It includes changes to the styling of buttons, adds try-catch block when fetching collections, and ensures that queries in a collection have the correct storage type when a collection is created. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe changes introduce improved error handling in the API service by adding a try-catch block to the Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/altair-app/src/app/modules/altair/services/api/api.service.tsOops! Something went wrong! :( ESLint: 8.18.0 ESLint couldn't find the plugin "@angular-eslint/eslint-plugin". (The package "@angular-eslint/eslint-plugin" was not found when loaded as a Node module from the directory "/packages/altair-app".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "@angular-eslint/eslint-plugin" was referenced from the config file in "packages/altair-app/.eslintrc.js#overrides[0]". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @imolorhe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request focuses on enhancing the handling of remote query collections within the Altair application. The primary goal is to allow exporting remote collections by addressing error handling and ensuring proper data management. I've added a try-catch block to the getCollection method in api.service.ts to handle potential errors when fetching collections from the server. Additionally, I've updated the QueryCollectionService to ensure that when a collection is added, its queries and the collection itself are explicitly marked with a storageType of 'local' and a workspaceId of WORKSPACES.LOCAL. Finally, I've updated the getCollectionTreeByCollectionId method to use getCollectionByID instead of getLocalCollectionByID to retrieve collections, and made some styling adjustments to the ant design button components.
Highlights
- Error Handling: Implemented a try-catch block in
ApiService.getCollectionto gracefully handle errors when fetching remote collections. - Data Management: Ensured that new collections and their queries are explicitly marked as 'local' with the appropriate
workspaceIdinQueryCollectionService. - Collection Retrieval: Modified
QueryCollectionService.getCollectionTreeByCollectionIdto use the genericgetCollectionByIDmethod. - Styling: Adjusted the styling of the ant design button components.
Changelog
- packages/altair-app/src/app/modules/altair/services/api/api.service.ts
- Added a try-catch block to the
getCollectionmethod to handle potential errors during remote collection fetching (lines 149-159).
- Added a try-catch block to the
- packages/altair-app/src/app/modules/altair/services/query-collection/query-collection.service.ts
- Modified the
addmethod to explicitly setstorageTypeto 'local' andworkspaceIdtoWORKSPACES.LOCALfor new collections (lines 131-132). - Added
storageType: 'local'to each query in the collection (line 122). - Updated
getCollectionTreeByCollectionIdto usegetCollectionByIDinstead ofgetLocalCollectionByID(line 705).
- Modified the
- packages/altair-app/src/scss/_ant-overrides.scss
- Added styling adjustments to the ant design button components (lines 43-66).
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
A remote collection's call,
Error's shadow starts to fall,
But code's embrace,
Finds a safer place,
And exports succeed for all.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request introduces the ability to export remote collections, enhancing the application's functionality. The changes include adding error handling to the getCollection method in api.service.ts, modifying the query-collection.service.ts to handle local and remote collections, and updating the styling for buttons in _ant-overrides.scss. Overall, the changes seem well-structured, but there are a few areas that could be improved.
Summary of Findings
- Error Handling in API Service: The addition of a try-catch block in
api.service.tsis good for handling errors when fetching collections. However, consider providing more informative error messages or using a more robust logging mechanism. - Redundant
storageTypeassignment: ThestorageTypeis being assigned in multiple places inquery-collection.service.ts. Consolidate these assignments to reduce redundancy and improve maintainability. - CSS Style Overrides: The CSS overrides in
_ant-overrides.scsscould be more specific to avoid unintended side effects on other components. Consider using more specific class names or nesting the styles within a more specific context.
Merge Readiness
The pull request is almost ready for merging. Addressing the redundant storageType assignments and refining the CSS overrides would improve the code quality. While the error handling is a good addition, consider enhancing the error messages for better debugging. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging. I recommend that the author address the comments before merging.
|
|
||
| return serverCollectionToLocalCollection(res); | ||
| } catch (e) { | ||
| console.error(e); |
There was a problem hiding this comment.
Consider adding more context to the error message, such as the collectionServerId, to aid in debugging.
| console.error(e); | |
| console.error(`Error fetching collection ${collectionServerId}:`, e); |
| return { | ||
| ...query, | ||
| storageType: 'local', | ||
| id: uuid(), | ||
| created_at: now, | ||
| updated_at: now, | ||
| }; |
There was a problem hiding this comment.
The storageType is being assigned here. It's also assigned later when adding to the storage. Consider consolidating this to avoid redundancy.
return {
...query,
id: uuid(),
created_at: now,
updated_at: now,
};| ...collection, | ||
| storageType: 'local', |
There was a problem hiding this comment.
The storageType is being assigned here as well. Consider consolidating the storageType assignment to a single location.
...collection,
workspaceId: WORKSPACES.LOCAL,
id: collectionId ?? uuid(),| .ant-btn:focus, | ||
| .ant-btn:hover { | ||
| color: var(--primary-color); | ||
| border-color: var(--primary-color); |
There was a problem hiding this comment.
These styles are quite broad and could potentially affect other buttons in the application unintentionally. Consider using a more specific class or nesting these styles within a more specific context to avoid unintended side effects.
.altair-app .ant-btn:focus,
.altair-app .ant-btn:hover {
color: var(--primary-color);
border-color: var(--primary-color);
}![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @imolorhe - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment explaining why the try-catch block was added in
ApiService.getCollection. - It looks like you're setting
storageType: 'local'in a few places - consider creating a constant for this value to avoid duplication.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Visit the preview URL for this PR (updated for commit 015177e): https://altair-gql--pr2813-imolorhe-allow-expor-7qpbocju.web.app (expires Mon, 28 Apr 2025 12:06:25 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 02d6323d75a99e532a38922862e269d63351a6cf |
Fixes
#2792
Checks
yarn test-buildChanges proposed in this pull request:
Summary by Sourcery
Add support for exporting and managing remote query collections in Altair
Bug Fixes:
Enhancements:
Chores:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Style